Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIDEA-168: component tests for globe #93

Merged
merged 25 commits into from
Nov 25, 2024

Conversation

david-mears-2
Copy link
Contributor

@david-mears-2 david-mears-2 commented Nov 11, 2024

Here are tests for the main testable behaviours of the globe component. As the component draws on a canvas, and the chart library doesn't expose functions like 'click', I couldn't find a way to simulate users actually clicking / hovering countries. So I mainly test the results of updating things in the store that the component has a watcher on, which correspond to user interactions.

Reference: https://www.amcharts.com/docs/v5/getting-started/integrations/jest/ I found that the list of libraries mentioned here was necessary.

NB: These component tests perform assertions about the return value of setUpChart(), which I call directly in the tests. This is a second-best kind of test, since it means we're testing a second chart object, rather than the one that was created automatically by the component setup script. I do this because I couldn't find a way to access that first chart object from here: component.vm.chart was always null.

For the reasons outlined in shapefiles.md (ie to avoid unit tests polluting the state of the geodata for each other), I introduce a script in this PR that reverses the winding order of the geodata (to be run when geodata is added to the repo) into the winding order used by amcharts (which is the reverse of WHO's winding order).

@david-mears-2 david-mears-2 changed the title wip: component tests for globe JIDEA-168: component tests for globe Nov 11, 2024
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 31.91296% with 751 lines in your changes missing coverage. Please review.

Project coverage is 37.15%. Comparing base (a63c6b7) to head (d682944).
Report is 26 commits behind head on main.

Files with missing lines Patch % Lines
...ata/4pc_downsampled_WHO_disputed_areas_22102024.js 0.00% 710 Missing ⚠️
assets/geodata/reverseWindingOrder.js 0.00% 39 Missing and 1 partial ⚠️
components/Globe.vue 91.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #93       +/-   ##
===========================================
+ Coverage    1.44%   37.15%   +35.70%     
===========================================
  Files          55       56        +1     
  Lines      146190   146233       +43     
  Branches      327      371       +44     
===========================================
+ Hits         2116    54334    +52218     
+ Misses     144057    91883    -52174     
+ Partials       17       16        -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@david-mears-2 david-mears-2 marked this pull request as ready for review November 15, 2024 12:14
Copy link
Contributor

@EmmaLRussell EmmaLRussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff. Mostly questions!


// Extract the map object from the file content
// eslint-disable-next-line no-eval
const map = eval(fileContent.replace("export default", ""));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you could do this as a dynamic import. But it doesn't really matter!

const updatedFileContent = `const map = ${JSON.stringify(map, null, 2)};\nexport default map;`;

// Save the updated content back to the file
fs.writeFileSync(filePath, updatedFileContent, "utf8");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it would be a little more cautious to write out to a different file from the input file.

@@ -0,0 +1,6 @@
module.exports = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this file needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. Might be worth sticking in a comment to that effect as I for one will definitely forget that.

Comment on lines 17 to 20
// NB: These component tests perform assertions about the return value of setUpChart(), which I call
// directly in the tests. This is a second-best kind of test, since it means we're testing a
// second chart object, rather than the one that was created automatically by the component
// setup script. I do this because I couldn't find a way to access that first chart object from here.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you set the return object as a value in the component and then access it via vm? setupChart does return the chart, but it doesn't look like we do anything with it (except in the test).

Or is the problem that setupChart isn't actually being called in the test (the globediv watch isn't being triggered?

Copy link
Contributor Author

@david-mears-2 david-mears-2 Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It runs setupChart twice (in the case of running a test) - once on setup and once when we call it directly from the test. (As confirmed by console logging)

What do you mean by setting it 'as a value'? When I access 'component.vm.chart' it is mysteriously undefined. (In fact, it's whatever we initialize 'chart' to, since component.vm.chart is 3 when we initialize chart to 3).

Anyway, I've now found that if I swap out 'mountSuspended' (@nuxt/test-utils) to 'mount' (@vue/test-utils) the problem goes away.

Comment on lines 35 to 36
expect(chart.get("rotationX")).toBe(-100);
expect(chart.get("rotationY")).toBe(-25);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments on these magic numbers would be nice too. -25 is amountToTileEarthUpBy? -100 is Thailand-centric default?

Comment on lines 71 to 72
expect(chart.series._values.length).toBe(9);
const gbrSeries = chart.series._values[8];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Series are sort of like layers? And here you're expecting the highlighted one to be the final one?

Copy link
Contributor Author

@david-mears-2 david-mears-2 Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a newly added series

Comment on lines 74 to 76
// Ensure that the name has been updated from 'United Kingdom of Great Britain and Northern Ireland' to 'United Kingdom'
// Name is used for tooltips.
expect(gbrSeries._settings.geoJSON.properties.name).toBe("United Kingdom");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do these updated names come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They come from the metadata:

 const modelCountryName = appStore.globeParameter?.options?.find(o => o.id === feature.id)?.label;


expect(component.vm.gentleRotateAnimation.stopped).toBe(true);

const gbrSeries = chart.series._values[8];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to make a helper to select what should be the highlighted series (always the last one?) as this crops up a couple of times,

Comment on lines 221 to 225
await waitFor(() => {
expect(gbrSeries._settings.fill).not.toBe(originalColor);
expect(chart.get("rotationX")).toBe(originalX);
expect(chart.get("rotationY")).toBe(originalY);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it should update the colour but shouldn't update the rotation. And by the time it updates the colour it would have updated the rotation if it was going to? So doing this as a waitFor is ok..?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the test is valid, but to make it more robust to any future changes to the component, I've added a line that expects the fill color to be the target color, not just 'different from the original'.

david-mears-2 and others added 7 commits November 19, 2024 17:35
Swapping mountSuspended for mount (from vue test utils) solved the issue
described in the deleted comment, where 'component.vm.chart' was always
returning the value with which the component initialized the 'chart' variable.
@david-mears-2
Copy link
Contributor Author

david-mears-2 commented Nov 21, 2024

Test is failing because of API container image not working at the moment dependent on #97

Copy link
Contributor

@EmmaLRussell EmmaLRussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, would just suggest adding a comment to the babel file.

@david-mears-2 david-mears-2 merged commit 889e207 into main Nov 25, 2024
6 checks passed
@david-mears-2 david-mears-2 deleted the jidea-168-globe-component-tests branch November 25, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants